Skip to content

Conversation

dnno
Copy link
Contributor

@dnno dnno commented Jul 7, 2025

Description

The Terraform AWS provider introduces the kms_key_identifier property for the "aws_cloudwatch_event_archive" resource. This is not yet set by the terraform-aws-eventbridge module

Motivation and Context

This enables users to set a custom kms key

Breaking Changes

No, not directly. Although I'm not yet sure how an existing archive reacts to re-keying. Might be worth mentioning.

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

@antonbabenko antonbabenko changed the title feat!: Set kms_key_identifier for EventBridge archives feat: Set kms_key_identifier for EventBridge archives Jul 9, 2025
Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just a few minor requests (such as resolving CI failures) before the merge.

@dnno dnno requested a review from antonbabenko July 9, 2025 07:39
@dnno dnno force-pushed the set-kms-key-identifier-for-archives branch from 7cba568 to b91a2a3 Compare July 9, 2025 07:49
main.tf Outdated

description = lookup(each.value, "description", null)
desired_state = lookup(each.value, "desired_state", null)
kms_key_identifier = var.kms_key_identifier
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be using lookup and fetch from each like the rest of arguments? I think it should in all places.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the kms key is specified in the module variables, not per pipe. I think it depends a bit how you would want that to be implemented. The current approach would mean, that pipes, archives and eventbus use the same key specified on the module variables. If you wanted to use different keys, you can set up individual modules for archive, pipes and eventbus, though. That's what I do currently, as well.

Does that work for you? Or do I miss something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

User may want to use different keys for different resources, so using lookup for each is a common pattern. Please update it, and example if necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, please review.

@dnno dnno requested a review from antonbabenko July 9, 2025 08:04
@antonbabenko
Copy link
Member

It is ok for now, I will update/fix examples later (in a couple of weeks)...

@antonbabenko antonbabenko merged commit 72fa89e into terraform-aws-modules:master Jul 9, 2025
13 of 15 checks passed
antonbabenko pushed a commit that referenced this pull request Jul 9, 2025
## [4.1.0](v4.0.0...v4.1.0) (2025-07-09)

### Features

* Set kms_key_identifier for EventBridge archives ([#175](#175)) ([72fa89e](72fa89e))
@antonbabenko
Copy link
Member

This PR is included in version 4.1.0 🎉

@dnno
Copy link
Contributor Author

dnno commented Jul 9, 2025

Thanks for your work, mate!

Copy link

github-actions bot commented Aug 9, 2025

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 9, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants